-
Notifications
You must be signed in to change notification settings - Fork 174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make tzif public API easier to use #2027
Conversation
@sffc Just a small oversight, I merged too soon before publishing and the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you also want to add a categories
key while you're at it?
Ah, I probably should, probably just I did just publish it, so I'd have to bump to 0.1.1, but that's probably not an issue. |
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
Hmm, actually, @sffc @Manishearth I want to get your opinion. I just tried using the crate externally and it's a bit awkward to use. It requires that you also use the There are a couple options:
This seems a lot nicer and easier to use, but it doesn't really give the flexibility of whether you want to use We could get fancy and expose our own enums or generics to still let users choose between I'd have to bump the version to Sorry, I should have tried using the crate as an external dependency locally before publishing it. Now I know for future crates. |
@sffc @Manishearth I went ahead and implemented option 2) in the comment above. PTAL I just made it as plug-and-play (simple) as possible for now. We can always revisit things later if needed. And I made sure to test it locally as a stand-alone dependency in another test crate: use tzif::TzifParser;
fn main() {
let data = TzifParser::parse_file("/usr/share/zoneinfo/America/Los_Angeles").unwrap();
println!("{:#?}", data);
} |
utils/tzif/src/lib.rs
Outdated
|
||
impl TzifParser { | ||
/// Parses a `TZif` file at the provided `path`. | ||
pub fn parse_file<P: AsRef<Path>>(path: P) -> Result<TzifData, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: Should these just be toplevel public functions (parse_tzif_file
and parse_posix_file
)?
Typically when I see a Parser
object I imagine a mutable object that holds state
current design is mostly fine though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's reasonable.
I'll make the changes.
Fixes #1000